-
Notifications
You must be signed in to change notification settings - Fork 235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat(cv_deploy): Expose list of targeted inactive devices if Workspace submission failed due to ResponseCode.INACTIVE_DEVICES_EXIST #4990
base: devel
Are you sure you want to change the base?
Conversation
Review docs on Read the Docs To test this pull request: # Create virtual environment for this testing below the current directory
python -m venv test-avd-pr-4990
# Activate the virtual environment
source test-avd-pr-4990/bin/activate
# Install all requirements including PyAVD
pip install "pyavd[ansible] @ git+https://github.com/alexeygorbunov/avd.git@issue_4038_improve_inact_devs_err#subdirectory=python-avd" --force
# Point Ansible collections path to the Python virtual environment
export ANSIBLE_COLLECTIONS_PATH=$VIRTUAL_ENV/ansible_collections
# Install Ansible collection
ansible-galaxy collection install git+https://github.com/alexeygorbunov/avd.git#/ansible_collections/arista/avd/,issue_4038_improve_inact_devs_err --force
# Optional: Install AVD examples
cd test-avd-pr-4990
ansible-playbook arista.avd.install_examples |
We should only warn if requested state for the workspace will not need to submit it. It can be useful to do builds and preview the changes even if devices are not streaming. |
fixed
|
@@ -99,6 +120,23 @@ async def verify_devices_in_cloudvision_inventory( | |||
device.serial_number = found_device_dict_by_hostname[device.hostname].key.device_id | |||
device.system_mac_address = found_device_dict_by_hostname[device.hostname].system_mac_address | |||
|
|||
if verify_streaming: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If feels like we are building more lists and iterating over everything again and again.
Could be just set the streaming flag during the inspection above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean to extend CVDevice
class to add streaming
attribute?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah that would be my suggestion. But if we decide to just submit no matter what, we don't even need to record this, but just set the force flag or not as given.
verify_streaming: bool = False, | ||
raise_if_inactive: bool = False, | ||
inactive_exception_class: type = CVInactiveDevices, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need all these extra options on this function. If you just gather the streaming state in here, you could move the invocation of the verify to the workspace submission logic where everything would make more sense. It would also mean we would not raise early (before even trying to build the workspace).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to confirm, after moving this streaming check over to submission phase - should we still raise (and avoid initiating submission) is we discover inactive devices targeted to configuration update when WS submission is not forced? Or should we just let submission to be initiated and let if fail (although we know in advance that it will fail)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know I said let's not try, but what if the device started streaming meanwhile. I think we should just submit and catch the error and provide a nice message suggesting the force option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the challenge here is that in contrast to the failing build (where we can fetch details of actual ws_id + build_id pair showing what exactly failed for which device) - failure of the WS submission does not provide these details (subscribe
to WorkspaceServiceStub
just returns iterator of Workspace
objects and they have no details about what exactly happened/failed, just an overall code
and status
, not telling exactly which devices were not streaming).
If we assume that streaming status can change from inactive
to active
between "verifying devices" step and step to submit WS - then it's probably fair to say that streaming status can change between failing WS submission and re-fetching device inventory (streaming) (if we decide to react to the failed submission postfactum). Maybe a possible way to accurately identify actual streaming state of the devices during WS submission is by subscribing to DeviceServiceStub
somehow for the whole duration of the submission phase and then correlating potential changes with exact WS submission timestamp. But not sure if this is not an overkill.
Maybe we can then indeed just fetch streaming status at the initial "verification" phase and then if WS submission fails - just assume that streaming didn't change between verification and submission phases
|
Change Summary
Verify streaming status of all devices targeted by
cv_deploy
and raise (and update Errors) or update Warnings (depending on theforce
parameter of the Workspace) if any inactive device caused Workspace submission failure.Related Issue(s)
Fixes #4038
Component(s) name
arista.avd.cv_deploy
Proposed changes
cv_client.get_inventory_devices
inverify_devices_in_cloudvision_inventory
already fetches actual state of all targeted devices including their streaming status. Add additional attribute_streaming
to describe streaming state ofCVDevice
s.forced
andinactive
devices were present prior to initiating submission - log warning message and append it to result.warnings as well:forced
,submit_result.code == ResponseCode.INACTIVE_DEVICES_EXIST
andinactive
devices were present prior to initiating submission - log warning message and raise exception:forced
,submit_result.code == ResponseCode.INACTIVE_DEVICES_EXIST
and but all devices were active prior to initiating submission - log warning message and raise exception without specifying actual devices (they might have change their streaming status between device verification and Workspace submission phases):How to test
cv_deploy
molecule tests targeting CI environmentcv_deploy with cv_submit_workspace_force = true
testcv_deploy with cv_submit_workspace_force = false
testChecklist
User Checklist
Repository Checklist